Fix response body #84
Conversation
defanator
requested review from
phantom-az
and removed request for
phantom-az
… can be
called by Nginx several times during request processing. And each time with
it own unique set of chained buf pointers.
For example, suppose a complete response consists of this chain of data:
A->B->C->D->E
Ngix may (and actually does, as verified by me in gdb) call body filter two
times like this:
handler(r, in = A->B->C)
handler(r, in = D->E), E has last_buf set
Current implementation delays feeding chain->buf to msc_append_response_body
until it comes upon a chain with buf->last_buf set. So we loose chain containing
A->B->C sequence. We must process body bufs as soon as we see them in body
handler otherwise we will not see them again.
N.B. You have PR owasp-modsecurity#84 pending. It goes further and fixes the problem when
a blocking decision is made after headers were sent. I intentionally retained
current (buggy) behavior to make my patch less intrusive and easier to review.
Besides owasp-modsecurity#84 impose an excessive memory usage due to a complete copy of all
bufs passed through body filter (we have sometimes 500K and more replies in our
applications) - I will elaborate on this in code review for owasp-modsecurity#84.
|
The proposed PR has a major drawback. It will cause excessive memory usage when handling large response bodies due to a complete copy of processed response bytes in ctx->temp_chain even when SecResponseBodyAccess is Off. As a quick work-around one may disable processing of bufs (and creating copies) if SecResponseBodyAccess is Off. |
| ngx_int_t data_offset = chain->buf->pos - chain->buf->start; | ||
| u_char *data = chain->buf->start; | ||
| msc_append_response_body(ctx->modsec_transaction, data, | ||
| chain->buf->end - data); |
There was a problem hiding this comment.
This is wrong. Actual data must be referenced by .pos and .last. See #104
|
Thanks for you comment, now I'm fix buffer processing. Unfortunely, now we are not have API to get value of directive SecResponseBodyAccess in connector |
|
@zimmerle valid concern about accessing various things like SecResponseBodyAccess from connectors. Is it something that could be achieved in the library on the cheap? |
|
That is indeed a valid concern. There are a few circumstances where the body is not considered or it is partially considered:
All those should checks are treated by the appendResponseBody. It just proceed with a copy if that is really necessary. Since the content is already a buffer (in some format), just a pointer is stacked. So, I don't expect to have a massive performance impact. I would avoid any decision like that to be taken under the connector side, unless it is extremely necessary. By design we want the connector to be as simple as possible, with no logic, simple bridging the content towards the library. With that, we expect to have easy integration with 3rd party software. Let me check the lib code to understand if it is doing what it meant to be doing and get back to you. |
|
Content complete disable is not being checked by appendResponseBody. Making a patch and get back to you with a new branch for testing. |
|
@turchanov, @defanator, @dennus do you mind to test it again? make sure to include the commit: owasp-modsecurity/ModSecurity@42a472a. It is now considering if the response body was enabled or disabled. |
…_filter.
A body filter function (ngx_http_modsecurity_body_filter in our case)
can be called by Nginx several times during request processing. And
each time with it own unique set of chained buf pointers.
For example, suppose a complete response consists of this chain of data:
A->B->C->D->E
Ngix may (and actually does, as verified by me in gdb) call body filter two
times like this:
handler(r, in = A->B->C)
handler(r, in = D->E), E has last_buf set
Current implementation delays feeding chain->buf to msc_append_response_body
until it comes upon a chain with buf->last_buf set. So we loose chain containing
A->B->C sequence. We must process body bufs as soon as we see them in body
handler otherwise we will not see them again.
N.B. You have PR #84 pending. It goes further and fixes the problem when
a blocking decision is made after headers were sent. I intentionally retained
current (buggy) behavior to make my patch less intrusive and easier to review.
Besides #84 impose an excessive memory usage due to a complete copy of all
bufs passed through body filter (we have sometimes 500K and more replies in our
applications) - I will elaborate on this in code review for #84.
|
@zimmerle, yes, I'm it tested it again. All ok, but unfortunely we still can't have a way to not starting processing response body, because descision makes in processResponseBody and we still must copyng all chunks and create chain copy. |
|
At first sight I was concerned about a copy of the data to be duplicated inside ModSecurity. Question: can we delivery the content to ModSecurity by pieces? without duplicate the chain? I am thinking in a good way to extract the information of a configuration-location in a good manner. Don't want to make it mandatory for the connector implementation, but, useful for optimizations. |
|
We call msc_append_response_body every time when receive next chunk of response body. But yes, whe hold response body in chain copy, because if we don't do this, nginx write filter will send to client body chunks before sending headers (which we hold before in header filter). If we don't hold headers, we cannot change any of them (for e.g., we cannot change response state code) because after it sending we get "header already sent" error. |
|
I've got your point, and it seems like the major performance problem is duo to this chain hold. That leads to a question: isn't this chain hold also necessary in order to have proper headers set in inspections on phases grater than 3? Regardless being inspecting the body or not. |
|
As far as I can see, this true - any modification on headers after it's sending to client throws error, that if we are trying to modify it after phase 3, we must hold a chain (except a case on 4 phase, when first call in msc_process_intervention in chunks processing cycle returns negative result). Another words, if we want to send modified headers, we must do this before sending a body). Maybe @defanator correct me. From another point a view, does msc_process_intervention may return non zero result before or after msc_process_response_body on phase 4, if SecResponseBodyAccess turned to off? If not, we may not to hold response body chain, because headers will never be modify. |
|
Can you double-check if that is still an issue after merging #165? |
|
This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days |
|
Hello, I don't know exactly why, but it seems that we're having the same issue for a few weeks. Modsecurity correctly catches and deny the request. But I fear that requests are still passed to the PHP upstream socket, as I have "500 error headers already sent". I wanted to know if it can be related to a (mis)configuration I made, as I'm unable to find the issue... Below are the 2 log lines: first is the catch, second is the 500. All Modsec catch will throw a Header already sent 500 after. Thing is, the ModSec logline contains parts of the 500 error text "while sending to client,". In the end the request is catched, logged, and so fail2ban can detect it. But I fear that the request is still passed to the PHP socket: as I have the headers already sent. Thanks EDIT: using |
|
What is your SecRequestBodyAccess value? |
|
@airween the value is ty! Nginx infos: |
…_filter.
A body filter function (ngx_http_modsecurity_body_filter in our case)
can be called by Nginx several times during request processing. And
each time with it own unique set of chained buf pointers.
For example, suppose a complete response consists of this chain of data:
A->B->C->D->E
Ngix may (and actually does, as verified by me in gdb) call body filter two
times like this:
handler(r, in = A->B->C)
handler(r, in = D->E), E has last_buf set
Current implementation delays feeding chain->buf to msc_append_response_body
until it comes upon a chain with buf->last_buf set. So we loose chain containing
A->B->C sequence. We must process body bufs as soon as we see them in body
handler otherwise we will not see them again.
N.B. You have PR owasp-modsecurity#84 pending. It goes further and fixes the problem when
a blocking decision is made after headers were sent. I intentionally retained
current (buggy) behavior to make my patch less intrusive and easier to review.
Besides owasp-modsecurity#84 impose an excessive memory usage due to a complete copy of all
bufs passed through body filter (we have sometimes 500K and more replies in our
applications) - I will elaborate on this in code review for owasp-modsecurity#84.
|
Apologies, I believe there is another dimension to this issue. I have observed that when the size of the response body crosses a particular threshold, even if the ModSecurity engine detects and throws a 403, the client still receives the complete request with a status of 200. For example, if I add a custom regex rule to detect the word "jolly" in the response, the client receives the response with a status 200, even though it is logged as 403 in the audit logs and 500 in the Nginx logs (due to headers already being sent). |
Complete response processing, I'm changed two tests in modsecurity.t, because now response validation may bring to errors like 403, 401, etc. But Nginx special responses with code < 400 returned only on phase 3